-
Notifications
You must be signed in to change notification settings - Fork 151
bugfix(savegame): Fix object list order after load #2161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
bugfix(savegame): Fix object list order after load #2161
Conversation
…nal save order Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Greptile Overview
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Implemented reverse-order save and backwards compatibility fix for old saves. Contains a style violation with single-line loop body |
| GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Implemented reverse-order save and backwards compatibility fix for old saves. Contains a style violation with single-line loop body |
Sequence Diagram
sequenceDiagram
participant Game as Game Save/Load
participant GL as GameLogic::xfer()
participant OL as Object List
participant Obj as Object
alt SAVE Mode (Version 10+/11+)
Game->>GL: xfer(XFER_SAVE)
GL->>OL: Traverse forward to find last object
loop For each object (reverse order)
GL->>Obj: getPrevObject()
GL->>Obj: xfer object data
end
Note over GL,Obj: Objects saved newest-first
else LOAD Mode
Game->>GL: xfer(XFER_LOAD)
loop For each saved object
GL->>Obj: Create new object
Note over Obj: prependToList() adds to head
GL->>Obj: xfer object data
end
alt Old Save (Version ≤9 or ≤10)
Note over GL,OL: Objects loaded in wrong order
GL->>OL: Reverse entire object list
loop Reverse linked list
OL->>Obj: Swap next/prev pointers
end
Note over GL,OL: Order corrected
else New Save (Version 10+/11+)
Note over GL,OL: Objects already in correct order
end
end
|
Nice. You'd be also able to test this by saving a game with two Command Centers, then pressing H and expecting to navigate to the same Command Center before and after loading a save. |
That's not related to this change, though the underlying issue is the same. You can uncomment this code to get the same behavior for GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/RTS/Team.cpp Lines 2742 to 2743 in a4482e0
|
|
Could reverse iterating during saving also fix this? |
I think so. That wouldn't work with already existing save games, though. Also, I think it requires changes to multiple functions. |
|
This is a good find.
I support this. Fixing the order of objects after creation is alright, but creating the objects in the correct order to begin with will be superior, because that will respect the right ordering even during creation, which hopefully is logically irrelevant, but technically more correct. |
|
The code actually get simpler if you do the work when saving. This will only work properly for new save games. GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp Line 4844 in 2c26a90
-> Object *lastObj = nullptr;
for (obj = getFirstObject(); obj; obj = obj->getNextObject())
{
lastObj = obj;
}
for( obj = lastObj; obj; obj = obj->getPrevObject() )New Object* getPrevObject() { return m_prev; }
const Object* getPrevObject() const { return m_prev; } |
|
Good damn single linked list 😄 |
|
Does it make sense to increase the xfer version for this? It would give us a way to distinguish new save games with inverse object list if we ever needed to. |
|
Yes can do. Is not strictly necessary but fair to do as you have described. |
|
I'd be inclined to do both, reverse the list on load if the xfer version is retail, save with correct order with new version and load as is if version is same or greater than new version. |
|
|
||
| // version | ||
| const XferVersion currentVersion = 9; | ||
| const XferVersion currentVersion = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume RETAIL_COMPATIBLE_XFER_SAVE is not strictly necessary? Retail game can still load these save games correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if an old game tries to load a new save it will just fail gracefully. I don't think this will be an issue - if someone asks a friend with TSH to create a .sav and share it with them maybe? But why would that ever happen? If we need to be able to load new saves on old game, we could use RETAIL_COMPATIBLE_XFER_SAVE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the way it would work is that version 10 isn't known by retail and isn't checked so it will load the list the same way it always did which will be the new correct order for saves saved after this version, the old incorrect order for save from before this. This new version checks on load so the order loaded will be correct for builds after this PR is merged no matter what version wrote the save.
|
|
||
| } | ||
|
|
||
| // TheSuperHackers @bugfix bobtista 27/01/2026 Reverse object list for old saves. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would label this change a fix, as they're not user-facing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct. See suggestion from Caball regarding comment tag.
There are a bunch of TODO's in the opening description.
Summary
Test plan
Todo: